Skip to content

Conversation

@MichaelChirico
Copy link
Member

There are few enough <- usages that I went ahead and made the switch, not that upstream {lintr} accommodates our choice of default operator.

There is still a pretty major issue with assignment_linter():

r-lib/lintr#2765

So I am still holding off activating it generally.

@MichaelChirico MichaelChirico added the code-quality Issues related to improving code quality/readability label Feb 19, 2025
@codecov
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.64%. Comparing base (770e80b) to head (3af2c2a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6825   +/-   ##
=======================================
  Coverage   98.64%   98.64%           
=======================================
  Files          79       79           
  Lines       14642    14642           
=======================================
  Hits        14444    14444           
  Misses        198      198           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Feb 19, 2025

Comparison Plot

Generated via commit 3af2c2a

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 4 minutes and 37 seconds
Installing different package versions 8 minutes and 17 seconds
Running and plotting the test cases 2 minutes and 22 seconds

@tdhock
Copy link
Member

tdhock commented Feb 20, 2025

ok for the change from <- to = but I don't understand what is the issue with linting? and why you want to hold off activating it generally?
I see lint-r failures that say we should use <- instead of = why? (I expect those failures should be suppressed because we want to use = instead of <- right?)

@MichaelChirico
Copy link
Member Author

what is the issue with linting?

Whoops, we still need to actively disable the default assignment_linter() (which requires <- or <<-). Fixed: 3af2c2a

We still comment out assignment_linter(operator="=") because of the linked {lintr} issue: we use <- a lot for implicit assignments, e.g.

if (!all(idx <- by.x %chin% nm_x)) {

The intended behavior is that implicit_assignment_linter() governs the style for implicit assignments:

implicit_assignment_linter = NULL,

@MichaelChirico
Copy link
Member Author

The remaining lints here are those fixed in #6284

@tdhock
Copy link
Member

tdhock commented Feb 20, 2025

ok, so I understand that there is some issue in lintr about "implicit" assignment versus not. (explicit?)
since the remaining lints are unrelated, I guess it is ok to merge?

Warning: file=R/data.table.R,line=1186,col=33,[seq_linter] Use seq_along(x) instead of seq_len(length(x)).
Warning: file=R/data.table.R,line=2383,col=30,[seq_linter] Use seq_along(x) instead of seq_len(length(x)).
Warning: file=R/data.table.R,line=2870,col=38,[seq_linter] Use seq_along(x) instead of seq_len(length(x)).

@MichaelChirico
Copy link
Member Author

since the remaining lints are unrelated, I guess it is ok to merge?

See comment above :)

@MichaelChirico MichaelChirico merged commit cbb918b into master Feb 20, 2025
10 of 11 checks passed
@MichaelChirico MichaelChirico deleted the assignment=eq branch February 20, 2025 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality Issues related to improving code quality/readability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants